Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206

sgeisler · 2020-04-07T20:51:15Z

This PR adds bitcoin_hashes as an optional dependency to implement the ThirtyTwoByteHash trait for the relevant hash types. It also adds a shortcut function Message::from_hashed_data<H: Hash>(&[u8]).

stevenroose

Looks ok so far. I'd like to look a bit closer in the situations where this trait is used and how this affects the UX as a rust-secp user.

I might also be tempted to have the feature be default.

stevenroose · 2020-04-07T22:36:32Z

Cargo.toml

@@ -46,6 +46,10 @@ rand_core = "0.4"
 serde_test = "1.0"
 bitcoin_hashes = "0.7"

+[dependencies.bitcoin_hashes]
+version = "0.7"
+optional = true


Any arguments here against making that default? Annoyingly someone that is depending on bitcoin and depends on secp and hashes using bitcoin::secp256k1 and bitcoin::hashes can't add features to the secp dependency. Well, that's a broader Cargo problem, but well.

I thought about this a lot 😅 (also because of features I might want to activate in secp256k1/bitcoin_hashes to not cause incompatibilities), turns out that cargo builds dependencies with the union of all the features activated if a dependency exists multiple times in the dependency graph (just tested it with a minimal example). So all they had to do was to specify secp256k1 as a dependency themselves and activate the feature.

On the other hand we could just activate that feature in bitcoin ~~and use it there too~~. After taking a closer look we don't seem to have any transaction signing code? At least we don't use secp256k1::Message anywhere.

Sure? It might be that we use an into() and that Message is never mentioned verbatim. I add a usage of this in the Bitcoin Signed Message PR.

stevenroose · 2020-04-07T22:39:10Z

src/lib.rs

+
+    /// Constructs a `Message` by hashing `data` with hash algorithm `H`
+    #[cfg(feature = "bitcoin_hashes")]
+    pub fn from_hashed_data<H: ThirtyTwoByteHash + bitcoin_hashes::Hash>(data: &[u8]) -> Self {


Hmm how is a method like this used? By explicitly providing the type param? That might be a bit awkward UX?

let msg = Message::from_hashes_data<sha256::Hash>(&my_data[..]);

Or is there another way?

Right, that's a bit awkward, but imo still better than having a separate method for each hash function. Given the confusion I should definitely document it better since using a type parameter in such a way is not too common. Alternatively I could just remove it, Message::from(sha256::Hash::hash(&my_data)) will do the same.

Just added an example to the docs. I personally find that, although a bit longer, the first version reads better.

elichai · 2020-04-09T13:13:28Z

I think this is interesting.
I would like to play with the ux of the function that hashes the data for you, maybe by passing in a hasher instead of just being generic over one.
Anyhow, I think an API that hashes for you is a safer API.

stevenroose

LGTM

stevenroose · 2020-04-19T19:01:56Z

src/lib.rs

+    #[cfg(feature = "bitcoin_hashes")]
+    pub fn from_hashed_data<H: ThirtyTwoByteHash + bitcoin_hashes::Hash>(data: &[u8]) -> Self {
+        let hash: H = bitcoin_hashes::Hash::hash(data);
+        Message::from(hash)


nit: What a missed chance for a cool one-liner!

<H as bitcoin_hashes::Hash>::hash(data).into()

Don't we all love Rust! :D

Oh yeah, that's neat, let me sneak it in :P

Hmm, didn't notice you have the trait imported

#[cfg(feature = "bitcoin_hashes")] use bitcoin_hashes::Hash;

So it could be shorted. But nvm that.

I actually avoid that on purpose. It collides with std::hash::Hash too often in my experience. EDIT: thinking about it: where is this import needed? 😄

stevenroose · 2020-04-19T19:02:07Z

src/lib.rs

+
+    /// Constructs a `Message` by hashing `data` with hash algorithm `H`
+    #[cfg(feature = "bitcoin_hashes")]
+    pub fn from_hashed_data<H: ThirtyTwoByteHash + bitcoin_hashes::Hash>(data: &[u8]) -> Self {


stevenroose

utACK 57526f8

thomaseizinger · 2020-04-24T08:43:19Z

I thought about the APIs in place here quite some time while implementing a PoC that uses the secp256k1 curve in ways other than just signing Bitcoin transactions and I found the Message API to be particularly constraining.

Here is a proposal for dealing with the whole ThirtyTwoByteHash and "how do we make rust-secp256k1 and bitcoin_hashes play nicely with each other" topic: #211

sgeisler · 2020-04-24T10:44:58Z

While your implementation looks technically sound and maybe even a bit more idiomatic too I'm not sure if we want to encourage to use arbitrary [u8;32]s to construct messages. Imo an API should make it easy to do the right thing and hard to do the wrong thing. Thinking about that this PR should also add a warning to from_slice that refers to the new API. See also this discussion.

If there is ever a common and sane need to sign anything other than a hash we can extend the API. ~~E.g. a Random([u8;32]) struct that can only be constructed using a cryptographically secure RNG and implements ThirtyTwoByteHash would be nice.~~ After thinking about it that's probably a bad idea.

thomaseizinger · 2020-04-24T13:30:42Z

Imo an API should make it easy to do the right thing and hard to do the wrong thing. Thinking about that this PR should also add a warning to from_slice that refers to the new API.

I tend to agree with that but it is kind of tricky depending on the abstraction level of your library (more low-level APIs tend to be unsafer in their usage). The fact that rust-bitcoin, rust-secp256k1 and bitcoin_hashes are three different crates put some artificial restrictions in place here.
We now have an optional feature to bitcoin_hashes in a crate that is only supposed to deal with an elliptic curve. That is kind of odd, no?
That is why I went with raw byte arrays as that is the lowest common denominator between those crates (assuming we want to keep the current split as is).

Currently, the from_slice function doesn't do any checks apart from the length. From that state, it seems pretty pointless to have as array length is something that can be enforced by the type system.

However, the trait documentation mentions things like all-zero messages and ones that overflow the group order. That sounds like we should actually be doing more checks in the from_slice constructor than what we are doing today?

sgeisler · 2020-04-24T13:50:02Z

Having a hashing crate as an optional dependency isn't odd at all since the only ~~sane~~ way to sign anything is to hash it first. This crate is also primarily a BTC crypto lib, so having a tighter integration with a bitcoin specific hash library isn't too bad imo. While secp256k1 the c library is quite low level this crate should have (and has afaik) the goal to be more use friendly and to have safe abstractions.

I see that yet another dependency to be updated isn't ideal, but it's a trade off I'm willing to make for a safe API. Alternatively there could be a separate crate secp256k1-safe that contains the ThirtyTwoByteHash trait and has both bitcoin_hashes and secp256k1 as dependencies and just pub uses the relevant structs from both. But that seems a bit over the top.

real-or-random · 2020-04-24T13:54:27Z

We now have an optional feature to bitcoin_hashes in a crate that is only supposed to deal with an elliptic curve. That is kind of odd, no?

Who said that it's supposed to deal with an elliptic curve? The README states "rust-secp256k1 is a wrapper around libsecp256k1, a C library by Pieter Wuille for producing ECDSA signatures using the SECG curve secp256k1".

So the main point of this library is to create (and verify) ECDSA signatures. And ECDSA relies on an elliptic curve and a hash function. People tend to forget about the hash function.

Having a hashing crate as an optional dependency isn't odd at all since the only sane way to sign anything is to hash it first.

Worse, it's actually not "signing" if you don't do the hash.

thomaseizinger · 2020-04-24T14:08:05Z

So the main point of this library is to create (and verify) ECDSA signatures. And ECDSA relies on an elliptic curve and a hash function. People tend to forget about the hash function.

That is a fair point. I guess something like this Digest trait could be useful as-well then the signing part wouldn't be limited to the hash functions defined in bitcoin_hashes.

In any case, this sounds to me like we should just directly remove from_slice? At least in its current state, its existence is inconsistent with the goal of providing safe abstractions.

elichai · 2020-04-26T07:30:54Z

That is a fair point. I guess something like this Digest trait could be useful as-well then the signing part wouldn't be limited to the hash functions defined in bitcoin_hashes.

I'm not a big fan of traits like Digest, because I then see people writing higher abstractions, while still making them generic over the trait and in the end it's the user who needs to decide the exact hash function, and there's a bunch of unsafe hash functions that implement this trait.
Worse than that, the user can accidentally "choose" a hash function because of the trait system.

I'm not sure what the best option here is.
IMHO libsecp should've been designed such that it hashes the data on signing/verifying instead of passing an already hashed data, although then some altcoins wouldn't like that because it would limit it to SHA2 only.

sgeisler · 2020-04-26T10:02:22Z

IMHO libsecp should've been designed such that it hashes the data on signing/verifying

The problem with that is (and why this crate shouldn't have an API like that either) that a naive implementation fn sign(sec_key: &SecretKey, msg: &[u8]) -> Signature would require the application to be able to keep the data that has to be signed completely in memory (hard for microcontrollers). If you use something like a signer that impls Write that signer had to keep the intermediate hashing state, an odd combination imo. So having different methods to acquire a hash and then giving it to a sign function while the type system enforces it's hash-ness seems to be at least a local API design optimum.

thomaseizinger · 2020-04-28T03:07:46Z

while the type system enforces it's hash-ness seems to be at least a local API design optimum.

We are not achieving this as long as from_slice is around.

sgeisler · 2020-04-28T11:07:16Z

I added documentation that makes it pretty clear to normally use the new API and only use from_slice if you know what you are doing.

apoelstra

utACK.

Is there any opposition to me merging this? I think this is roughly the API we had in mind when we created the ThirtyTwoByteHash, and I think having bitcoin_hashes be an optional non-default dependency of rust-secp is the least ugly way to manage the dependency tree.

elichai

Sure, Why not. as Andrew said this is probably the best compromise

sgeisler force-pushed the 2020-04-hashes branch from f9b202a to 80dd5ec Compare April 7, 2020 22:13

stevenroose reviewed Apr 7, 2020

View reviewed changes

sgeisler force-pushed the 2020-04-hashes branch 2 times, most recently from 75291bf to c905de2 Compare April 8, 2020 10:28

real-or-random mentioned this pull request Apr 9, 2020

Allow all-zero messages #207

Merged

stevenroose previously approved these changes Apr 19, 2020

View reviewed changes

sgeisler dismissed stevenroose’s stale review via 57526f8 April 19, 2020 21:07

sgeisler force-pushed the 2020-04-hashes branch from c905de2 to 57526f8 Compare April 19, 2020 21:07

stevenroose previously approved these changes Apr 20, 2020

View reviewed changes

sgeisler dismissed stevenroose’s stale review via 1c02050 April 28, 2020 10:51

sgeisler added 2 commits May 14, 2020 18:24

Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash

0d85753

Document proper usage of from_slice

8979a93

sgeisler force-pushed the 2020-04-hashes branch from 1c02050 to 0d85753 Compare May 14, 2020 16:27

apoelstra approved these changes May 23, 2020

View reviewed changes

elichai approved these changes May 24, 2020

View reviewed changes

apoelstra merged commit a5147bb into rust-bitcoin:master May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206

Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206

sgeisler commented Apr 7, 2020

stevenroose left a comment

stevenroose Apr 7, 2020

sgeisler Apr 8, 2020

stevenroose Apr 19, 2020

stevenroose Apr 7, 2020

sgeisler Apr 8, 2020

sgeisler Apr 8, 2020

stevenroose Apr 19, 2020

elichai commented Apr 9, 2020

stevenroose left a comment

stevenroose Apr 19, 2020

sgeisler Apr 19, 2020

stevenroose Apr 20, 2020

sgeisler Apr 20, 2020 •

edited

stevenroose Apr 19, 2020

stevenroose left a comment •

edited

thomaseizinger commented Apr 24, 2020 •

edited

sgeisler commented Apr 24, 2020 •

edited

thomaseizinger commented Apr 24, 2020

sgeisler commented Apr 24, 2020 •

edited

real-or-random commented Apr 24, 2020 •

edited

thomaseizinger commented Apr 24, 2020

elichai commented Apr 26, 2020

sgeisler commented Apr 26, 2020

thomaseizinger commented Apr 28, 2020

sgeisler commented Apr 28, 2020

apoelstra left a comment

elichai left a comment •

edited

Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206

Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206

Conversation

sgeisler commented Apr 7, 2020

stevenroose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Apr 9, 2020

stevenroose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgeisler Apr 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose left a comment • edited

Choose a reason for hiding this comment

thomaseizinger commented Apr 24, 2020 • edited

sgeisler commented Apr 24, 2020 • edited

thomaseizinger commented Apr 24, 2020

sgeisler commented Apr 24, 2020 • edited

real-or-random commented Apr 24, 2020 • edited

thomaseizinger commented Apr 24, 2020

elichai commented Apr 26, 2020

sgeisler commented Apr 26, 2020

thomaseizinger commented Apr 28, 2020

sgeisler commented Apr 28, 2020

apoelstra left a comment

Choose a reason for hiding this comment

elichai left a comment • edited

Choose a reason for hiding this comment

sgeisler Apr 20, 2020 •

edited

stevenroose left a comment •

edited

thomaseizinger commented Apr 24, 2020 •

edited

sgeisler commented Apr 24, 2020 •

edited

sgeisler commented Apr 24, 2020 •

edited

real-or-random commented Apr 24, 2020 •

edited

elichai left a comment •

edited